-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add DataDriftTrigger: support one Evidently metric #409
Add DataDriftTrigger: support one Evidently metric #409
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jingyi! I did not have big comments, mostly smaller code-style changes and documentation issues. I think it might still be sensible to merge the downloader model thing first instead of extracting it later (probably overall less work) but we can do it however you prefer. As mentioned on Element, let's discuss how you proceed with the other TODOs. We definitely need the unit tests in this PR before merging :)
modyn/supervisor/internal/pipeline_executor/pipeline_executor.py
Outdated
Show resolved
Hide resolved
modyn/supervisor/internal/triggers/model_downloader/model_downloader.py
Outdated
Show resolved
Hide resolved
modyn/supervisor/internal/triggers/model_downloader/model_downloader.py
Outdated
Show resolved
Hide resolved
✅ Result of Pytest Coverage---------- coverage: platform linux, python 3.12.3-final-0 -----------
|
Thanks for the work. Let me know when I should review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jingyi, LGTM with the exception of one small abstractmethod thing. And I would like @XianzheMa to double check the changes on the triggering indices since we just changed some stuff there :) After he also approves and the abstract thing is fixed and CI runs through, we can merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and can you address the small changes I requested before merging! Thanks!
modyn/supervisor/internal/pipeline_executor/pipeline_executor.py
Outdated
Show resolved
Hide resolved
modyn/supervisor/internal/pipeline_executor/pipeline_executor.py
Outdated
Show resolved
Hide resolved
…gers_within_batch. Add back persist_pipeline_log()
This is a clean version of PR#367. 1. Add DataDriftTrigger class to supervisor. Supports one configurable Evidently metric. Launches drift detection every N new data points. Data used in detection are data trained in the previous trigger and all the untriggered new data. 2. Update Trigger interface. `Trigger.inform()` returns a Generator instead of List. 3. Add a generic ModelDownloader in supervisor. 4. Add example pipelines using DataDriftTrigger. 5. Add Evidently to pylint known third party. 6. Change ModelDownloader to embedding encoder utils. The downloader sets up and returns the model. The DataDriftTrigger owns the model. Future 1. Support multiple configurable Evidently metric. #416 2. Support Alibi-Detect. #414 3. Support custom embedding encoder. #417 4. Support different windowing for detection data, e.g. compare with all previously trained data. #418 5. Common DataLoaderInfo #415
This is a clean version of PR#367.
Trigger.inform()
returns a Generator instead of List.Future